support parallel snapshotting for uuid partitioning#4482
Conversation
e7d0d7c to
9b26c4a
Compare
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
❌ Test FailureAnalysis: The failures are a real bug in the PR's new partitioning logic — Test_Partition_Key_Empty fails with a deterministic, reproducible assertion ("expected no partitions to be created, but 1 was") on both CH and CH_Cluster variants, alongside the PR's own new partition-snapshot tests timing out in setup. |
❌ Test FailureAnalysis: Deterministic partition-key test failures (e.g. "expected no partitions to be created, but was 1") reproduce identically across all three MySQL matrix jobs and hit exactly the partition-key feature this "string-partitioning" PR modifies, indicating a real bug rather than flakiness. |
9b26c4a to
af27673
Compare
❌ Test FailureAnalysis: The PR's own new feature tests (Test_MySQL_String_Partition_Key_*) deterministically fail with "UNEXPECTED STATUS TIMEOUT STATUS_SETUP" identically across all three MySQL flavors, indicating the branch's new string/UUID partitioning setup logic is stuck in mirror setup — a real bug, not a flaky failure. |
af27673 to
acc33e8
Compare
acc33e8 to
dd731c6
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
| ) | ||
|
|
||
| // AddUuidStringPartitionsWithRange splits a UUID watermark range into partitions. | ||
| func (p *PartitionHelper) AddUuidStringPartitionsWithRange( |
There was a problem hiding this comment.
Trying to scope the PR to MySQL only so intentionally using a separate method here that gets explicitly called by mysql connector.
AddPartitionsWithRange is used by Postgres as well so enabling it for PG (e.g. when a custom string column is selected as the watermark column) requires more code changes and e2e testing.
There was a problem hiding this comment.
Would it make sense to move this method to an utils package within the MySQL connector subtree to prevent accidental usage? Or at least add a comment? I can imagine a happy agent or engineer thinking oh, this is convenient
There was a problem hiding this comment.
Sure, can move it to mysql for now and bring it back when we are ready to make it generic.
| ) | ||
|
|
||
| // AddUuidStringPartitionsWithRange splits a UUID watermark range into partitions. | ||
| func (p *PartitionHelper) AddUuidStringPartitionsWithRange( |
There was a problem hiding this comment.
Would it make sense to move this method to an utils package within the MySQL connector subtree to prevent accidental usage? Or at least add a comment? I can imagine a happy agent or engineer thinking oh, this is convenient
| maxV string | ||
| expectedCase *regexp.Regexp | ||
| }{ | ||
| {"lower", "018f6e7a-1b2c-7def-8a3b-1c2d3e4f5a6b", "f47ac10b-58cc-4372-a567-0e02b2c3d479", UuidLowerRe}, |
There was a problem hiding this comment.
I think it would make sense to add cases for the non-happy path cases just to confirm nothing crash. e.g:
lower:"not an uuid" upper: "018f6e7a-1b2c-7def-8a3b-1c2d3e4f5a6b"
lower: "f47ac10b-58cc-4372-a567-0e02b2c3d479" upper:"018f6e7a-1b2c-7def-8a3b-1c2d3e4f5a6b"
...
There was a problem hiding this comment.
oops, had a separate test for this, but looks like i accidentally deleted them when addressing a merge conflict.
There was a problem hiding this comment.
added non-happy path testing for buildUuidStringPartitions:
func TestBuildUuidStringPartitionsInvalid(t *testing.T) {
cases := []struct {
name string
minV string
maxV string
}{
{"arbitrary string", "apple", "banana"},
{"inverted", "f47ac10b-58cc-4372-a567-0e02b2c3d479", "018f6e7a-1b2c-7def-8a3b-1c2d3e4f5a6b"},
{"too short", "018f6e7a-1b2c", "f47ac10b-58cc"},
{"too long", "018f6e7a-1b2c-7def-8a3b-1c2d3e4f5a6bb", "f47ac10b-58cc-4372-a567-0e02b2c3d4799"},
}
const numPartitions = 32
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
_, err := buildUuidStringPartitions(tc.minV, tc.maxV, hexCasingLower, numPartitions)
require.Error(t, err)
})
}
}
Failure here is intentional since it indicates a code error on our end if buildUuidStringPartitions ends up processing invalid uuids. They should be caught by detectUuidWithHexCasing, which also test for invalid cases in TestDetectUUIDCase. There's also an e2e test Test_MySQL_String_Partition_Key_Arbitrary_FullTable to make sure that we default to full table partition. So we should be good on coverage now.
| // Because only the bounds are examined, the classification can be wrong in | ||
| // the following rare cases: | ||
| // - non-UUID rows can exist between UUID-shaped min/max bounds; | ||
| // - non-boundary UUIDs contain a mix of upper and lower case |
There was a problem hiding this comment.
Is it not possible to normalize the string values both at this partition computation level and at query level so at least for parallel snapshotting we are case insensitive?
There was a problem hiding this comment.
We can maintain correctness as long as the min/max values (in the first and last partitions) use the original casing (which is the case today). However, if table collation is case-sensitive, it can lead to data skew.
Skew example: say we have min/max uuids A0000..... and e0000...; let's say we normalize the computed boundaries to lower case and got the following bounds:
- P1 [A0000…, accccc…)
- P2 [accccc…, b99999…)
- P3 [b99999…, c66666…)
- P4 [c66666…, d33333…)
- P5 [d33333…, e0000…]
This means P1 would cover all the upper case uuids, leading to potential skew if table was recently switched to lower case.
Another skew example where we have mixed casing within an UUID: say we have min/max uuid A000a... and E000e...; and again we normalize the computed boundaries to lower case and got the following bounds
- P1 [A000a…, accccc…)
- P2 [accccc…, b99999…)
- P3 [b99999…, c66666…)
- P4 [c66666…, d33333…)
- P5 [d33333…, E000e…]
In this case all data ends up in the first partition, and the other computed partition boundaries return empty results.
We could get a bit smarter with what default casing to use, but it just adds more code complexity. Hence I was thinking we can treat mix-case as arbitrary string for more even partitioning.
There was a problem hiding this comment.
Oh wait, perhaps by normalize at computation/query level you probably mean something like where lower(<uuid_col>) >= <start_value_lower_case> and lower(<uuid_col>) < <end_value_lower_case>. This would work but cannot be done cheaply since it can't take advantage of the index, so we end up with full table scans.
There was a problem hiding this comment.
Oh wait, perhaps by normalize at computation/query level you probably mean something like where lower(<uuid_col>) >= <start_value_lower_case> and lower(<uuid_col>) < <end_value_lower_case>.
Yes, that's what I meant actually.
since it can't take advantage of the index, so we end up with full table scans.
Great point! I hadn't considered the index!
36fd325 to
be8cb0c
Compare
- Add end_inclusive to StringPartitionRange proto (aligns with MySQL PR PeerDB-io#4482 adding the same field; avoids future merge conflict) - Replace sentinel-string approach with end_inclusive=true on the last string partition, using $lte instead of $lt for its MongoDB filter - Sort string _id samples via MongoDB $sort stage instead of Go sort, preserving the collection's collation order - Filter boundary samples by exact equality instead of <=/>= comparison, trusting MongoDB's min/max guarantees - Add e2e tests for string and numeric _id partitioned snapshot flows - Document the double _id gap limitation on numericPartitions (follow-up)
- Add end_inclusive to StringPartitionRange proto (aligns with MySQL PR PeerDB-io#4482 adding the same field; avoids future merge conflict) - Replace sentinel-string approach with end_inclusive=true on the last string partition, using $lte instead of $lt for its MongoDB filter - Sort string _id samples via MongoDB $sort stage instead of Go sort, preserving the collection's collation order - Filter boundary samples by exact equality instead of <=/>= comparison, trusting MongoDB's min/max guarantees - Add e2e tests for string and numeric _id partitioned snapshot flows - Document the double _id gap limitation on numericPartitions (follow-up)
Add parallel snapshotting support for MySQL when default primary key is a UUID string column.
We best-effort detect uuid by evaluating the min and max values in the table; if both min and max value match uuid format (and have a consistent hex casing), we can guarantee correctness (see description for
DetectUuidWithHexCasingfor a bit more details). Original plan was to sample first and last N records, but it doesn't add a ton of value here, so just checking min/max values for simplicity.If string column is deemed non-UUID, we fallback to full table partition for now; will follow-up with partitioning for arbitrary string.
Testing: add unit and integration tests to ensure uuids PKs are parallel snapshotted while arbitrary string PKs still correctly replicate with full table partition.
Fixes: DBI-868